Remove SPIAwareTrait and adopt TestScoping in its place #901
Conversation
stmontgomery
added
enhancement
stmontgomery
requested review from
grynspan,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci please test |
stmontgomery
changed the title
| /// This trait is recursively applied: if it is applied to a suite, any | ||
| /// parameterized tests or test suites contained in that suite are also | ||
| /// serialized (as are any tests contained in those suites, and so on.) | ||
| /// If this trait is applied to a suite, any test functions or test suites |
There was a problem hiding this comment.
It's important to note that parameterized test functions are serialized (as opposed to just function A, function B, function C get serialized.)
There was a problem hiding this comment.
I'll try to tweak the wording more. What I was attempting to clarify with this re-wording is that regular, non-parameterized test functions within a suite which has .serialized are also serialized alongside any sub-suites which are their peers. I found the existing wording a bit unclear about the handling of non-parameterized test functions.
There was a problem hiding this comment.
I recall this being difficult to get right the last time too—might want to ping @iamleeg & friends for assistance?
There was a problem hiding this comment.
I've pushed a new phrasing in the mean time
stmontgomery
changed the title
|
@swift-ci please test |
…ce it wasn't experimental
Sources/Testing/Running/Runner.swift
Outdated
| } | ||
| } | ||
|
|
||
| if let step = stepGraph.value, case .run = step.action { | ||
| await Test.withCurrent(step.test) { | ||
| _ = await Issue.withErrorRecording(at: step.test.sourceLocation, configuration: configuration) { | ||
| _ = await Issue.withErrorRecording(at: step.test.sourceLocation, configuration: _configuration) { |
There was a problem hiding this comment.
Should we update withErrorRecording to just pull the current task-local value? Do we ever use it in a way that needs to take any other value?
There was a problem hiding this comment.
Given where I landed with other changes I don't think I'll need this right now
There was a problem hiding this comment.
But I agree in principle we could make that utility infer the current configuration, just not sure it's needed for this PR
|
@swift-ci please test |
…ot to avoid decoding error in clients (#1023) This fixes a regression in the encoding of the `Runner.Plan` snapshot types, which can manifest when using Xcode 16. ### Motivation: The `isParallelizationEnabled` property was deprecated and changed from a stored property to a derived one in #901. That caused the value to no longer be encoded in `Runner.Plan.Action.RunOptions`, which can cause decoding errors in versions of Xcode which expect it to still be present. ### Modifications: - Manually implement `Codable` conformance for the affected type to begin including a hardcoded value. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated. Fixes rdar://146284519
This is a follow-on from #733 which introduced the Test Scoping Traits feature. As that proposal indicated, the
SPIAwareTraitSPI protocol is no longer necessary, so this PR removes it and adoptsTestScopingin its place.Motivation:
Adopting Test Scoping Traits for the purpose which
SPIAwareTraitpreviously served simplifies a lot of runner and planning logic.Modifications:
SPIAwareTraitand related SPI.TestScopinginParallelizationTraitinstead.ParallelizationTraitto not always have the valuetrueforisRecursive. Test Scoping Traits makes this no longer necessary: the invocation of children of any suites which have.serializedis scoped to have parallelization disabled.isParallelizationEnabledon theRunner.Plan.Action.RunOptionstype. For now, I kept theRunOptionsstruct, since it could be useful in the future and it's SPI.Runnerfor propagating ancestor steps, and refactored slightly to make many thingsstatic. This is to help ensure certain functions don't access theself.configurationproperty when instead they should be accessingConfiguration.current.Result:
No functional change, but implementation is simplified.
Checklist: